-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(NODE-6350): add typescript support to client bulkWrite API #4257
base: main
Are you sure you want to change the base?
Conversation
f31fc03
to
8b309e2
Compare
evergreen refresh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot this is my PR.. LGTM! @durran
// We do not need schema type information past this point ("as any" is fine) | ||
return await new ClientBulkWriteExecutor( | ||
this, | ||
models as any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the executor just take a ReadonlyArray<ClientBulkWriteModel<Document>>
instead? I think ReadonlyArray<ClientBulkWriteModel> will always be assignable and we won't need as any
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
models
still isn't assignable
Argument of type 'readonly ClientBulkWriteModel<SchemaMap>[]' is not assignable to parameter of type 'readonly ClientBulkWriteModel<Document>[]'.
Type 'ClientBulkWriteModel<SchemaMap>' is not assignable to type 'ClientBulkWriteModel<Document>'.
Type 'ClientUpdateOneModel<SchemaMap[keyof SchemaMap]> & { namespace: keyof SchemaMap; }' is not assignable to type 'ClientBulkWriteModel<Document>'.
Type 'ClientUpdateOneModel<SchemaMap[keyof SchemaMap]> & { namespace: keyof SchemaMap; }' is not assignable to type 'ClientUpdateOneModel<any> & { namespace: string; }'.
Types of property 'update' are incompatible.
Type 'Document[] | UpdateFilter<SchemaMap[keyof SchemaMap]>' is not assignable to type 'Document[] | UpdateFilter<any>'.
Type 'UpdateFilter<SchemaMap[keyof SchemaMap]>' is not assignable to type 'Document[] | UpdateFilter<any>'.
Type 'UpdateFilter<SchemaMap[keyof SchemaMap]>' is missing the following properties from type 'Document[]': length, pop, push, concat, and 29 more.
I think passing down the parameterization is the "correct" way but it has no value because we don't edit or depend on any schema type information.
'db.stores': Store; | ||
}; | ||
|
||
expectError<ClientBulkWriteModel<MongoDBSchemas>>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we wanted this to compile but to fallback to no type safety.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe we can make it work that way if the keys of the SchemaMap are plain strings then you can't select a schema 🤔 But this test is about preventing namespaces typos, not the enforcement of the Filter type. Seems like a nice feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the keys are strings, can we just fallback to Document?
If I'm understanding correctly, the current API requires users to either completely strictly type their bulkWrite
api (map all namespaces to all schemas) or default to document - there's no in between.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the in-between use case you're thinking of?
If you add [key: string]: Document
to your SchemaMap you remove type assertion but also typo checking, so you revert to the default experience
Can you write out the TS you're imagining?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect users to be able to do:
const schema = {
'foo.bar': { name: string }
}
// fine
client.bulkWrite<schema>([
{ name: 'insertOne', namespace: 'foo.bar', document: { name: 'bailey' } }
])
// also fine
client.bulkWrite<schema>([
{ name: 'insertOne', namespace: 'foo.other-ns', document: { name: 'bailey' } }
])
// also fine
client.bulkWrite<schema>([
{ name: 'insertOne', namespace: 'foo.bar', document: { name: 'bailey' } },
{ name: 'insertOne', namespace: 'foo.other-ns', document: { name: 'bailey' } }
])
// error - type checking on the supplied namespaces
client.bulkWrite<schema>([
{ name: 'insertOne', namespace: 'foo.bar', document: { name: 3 } }, // error
{ name: 'insertOne', namespace: 'foo.other-ns', document: { name: 'bailey' } }
])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the example, but I was wondering what the implementation for that would look like? That behavior would remove checking for namespace typos right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From standup: checking the namespaces for typos is incompatible with making plain strings map to Document, so I don't think we want to do this.
I did look into this though, fell short of solution, I was toying around with deploying something like this:
type IsStringLiteral<T> = T extends string
? string extends T
? false
: true
: false;
But I think Namespace is always a string literal since it comes from keyof
evergreen retry |
Description
What is changing?
Adds namespace to schema mapping type
Adds tests/examples to our tsd tests
Some important choices:
ClientBulkWriteModel
is exported on its own and is not an array type. We can offer this seperate from the client API so annotations can be used to get this string to schema behavior (but I would advocate for putting it directly into the API)Is there new documentation needed for these changes?
Yes, I think the API doc on client.bulkWrite should explain how to use the TS a bit. I started in the TS doc for
ClientBulkWriteModel
We mostly use "inline" models to demo the API, however, this TS change does not cause friction in wrapping this API, it does have some considerations though, if you make a helper that returns InsertOneModels you will want the return type to still preserve the type of "namespace" where appropriate to get autocomplete and assertions. -- we should write a small example of this.
What is the motivation for this change?
Users of this method can get thier schema asserted on the various bulkWrite models making sure input data is not adjusted accidentally or that filters aren't written in a way that doees not match the data format at all.
Release Highlight
New client bulk write API supported
A new bulk write API on the
MongoClient
is now supported for users on server versions 8.0 and higher.This API is meant to replace the existing bulk write API on the
Collection
as it supports a bulkwrite across multiple databases and collections in a single call.
Usage
Users of this API call
MongoClient#bulkWrite
and provide a list of bulk write models and options.The models have a structure as follows:
Insert One
_id
field is provided in the document, the driver will generate a BSONObjectId
automatically. *
Update One
Update Many
Replace One
Delete One
Delete Many
Example
Below is a mixed model example of using the new API:
The bulk write specific options that can be provided to the API are as follows:
ordered
: Optional boolean that indicates the bulk write as ordered. Defaults to true.verboseResults
: Optional boolean to indicate to provide verbose results. Defaults to false.bypassDocumentValidation
: Optional boolean to bypass document validation rules. Defaults to false.let
: Optional document of parameter names and values that can be accessed using $$var. No default.The object returned by the bulk write API is:
Error Handling
Server side errors encountered during a bulk write will throw a
MongoClientBulkWriteError
. This errorhas the following properties:
writeConcernErrors
: Ann array of documents for each write concern error that occurred.writeErrors
: A map of index pointing at the models provided and the individual write error.partialResult
: The client bulk write result at the point where the error was thrown.Schema assertion support
Notice how authorName is type checked against the
Book
type because namespace is set to"db.books"
.Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript